Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement pluggable session store and make it threadsafe #12

Merged
merged 4 commits into from
Dec 11, 2016

Conversation

kirsle
Copy link
Member

@kirsle kirsle commented Dec 9, 2016

This implements the pluggable session storage feature, similar to RiveScript-Python PR #32.

The new package rivescript-go/sessions defines the SessionManager interface for managing user variables, and the package rivescript-go/sessions/memory implements the default in-memory manager.

⚠️ Incompatible Changes ⚠️

This changes some of the API functionality. After merging, the library version will be incremented to v0.1.0 accordingly.

The rivescript.New() Constructor

The constructor for rivescript.New() has been updated to accept a config object. Example:

import (
    "github.com/aichaos/rivescript-go"
    "github.com/aichaos/rivescript-go/config"
    "github.com/aichaos/rivescript-go/sessions/memory"
)

func main() {
    // If you want to specify your own config. The zero options
    // have sensible defaults for the most part, so you don't need
    // to specify every key.
    bot := rivescript.New(&config.Config{
        Debug: false, // default
        UTF8: true, // default false
        Strict: true, // default false
        Depth: 50, // default 50 if not defined or <= 0
        SessionManager: memory.New(), // default if not defined
    }

    // For convenience, there are two shortcuts
    bot := rivescript.New(config.Basic())
    bot := rivescript.New(config.UTF8())

    // Or if you don't care you can have all the defaults (this disables strict mode!)
    bot := rivescript.New(nil)
}

The "basic" config profile has Strict=true and all the other defaults, and the UTF8 profile has that, plus UTF8=true.

Other Functions

  • SetDepth() and GetDepth() now use a uint instead of an int
  • GetUservars() and GetAllUservars() return *session.UserData objects instead of map[string]string for the user data.
  • ThawUservars() now takes a sessions.ThawAction instead of a string, with valid values being Thaw, Discard or Keep (all constants in the sessions package).

Thread Safety

This PR will fix #10 because the default MemoryStore implementation of the SessionManager uses a mutex to lock concurrent access to the underlying user data maps.

Additionally, a common mutex rs.cLock protects map access to global variables, bot variables, substitutions, macro handlers and subroutines when using the API methods to modify these. TODO is to use this mutex when a !Definition command is parsed or a <bot> or <env> tag is handled, but these are edge cases and not a high priority right now.

The following Go script verified that the maps are thread safe (this code gave a concurrent read error before implementing this change):

package main

import (
	"fmt"
	"sync"
	"time"

	rivescript "github.com/aichaos/rivescript-go"
	"github.com/aichaos/rivescript-go/config"
)

var done int
var lock sync.Mutex

func main() {
	bot := rivescript.New(config.Basic())
	bot.LoadDirectory("eg/brain")
	bot.Stream(`
		! var test = hello
		! global test = hello

		+ update test
		- <bot test=world><env test=hello>OK: <bot test>
	`)
	bot.SortReplies()

	for i := 0; i < 10; i++ {
		go ConcurrentTest(fmt.Sprintf("user%d", i), bot)
	}

	for done < 10 {
		time.Sleep(1)
	}
}

func ConcurrentTest(username string, bot *rivescript.RiveScript) {
	fmt.Println("Concurrent test")
	for i := 0; i < 100; i++ {
		_ = bot.Reply(username, "hello bot")
		_ = bot.Reply(username, "update test")
	}
	fmt.Println("Done")

	lock.Lock()
	done++
	lock.Unlock()
}

@kirsle kirsle mentioned this pull request Dec 9, 2016
@kirsle kirsle force-pushed the feature/session-store branch from e57563f to 85292d5 Compare December 10, 2016 00:03
@kirsle kirsle merged commit 7f29a68 into master Dec 11, 2016
@kirsle kirsle deleted the feature/session-store branch December 11, 2016 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Library not thread safe
1 participant